Introduction of AttemptClass and PaymentClass (Issue #20) by sebulino · Pull Request #24 · renepickhardt/pickhardtpayments

sebulino · 2022-05-20T15:41:14Z

Extraction of Attempt and Payment.
A Payment is a central object in the library, and in particular in the simulation. This is why it "deserves" its own Class.
Each onion is recorded as an Attempt and registered with the respective Payment. This not only allows for statistics for each Payment object but also collects all attempts where an amount could successfully flow from sender to receiver.

refactoring of SyncSimulatedPaymentSession has been done as well as a first version of the documentation of the two classes.

first stub of a method as described in renepickhardt#16 Still needs to be tested

… routed successfully

This reverts commit 8dffa28.

renepickhardt

Thanks a lot! Despite me having a few question mark, comments and suggestions (I guess it is much easier to question code rather than writing it) it is a real pleasure reading your code. I have the feeling you write much cleaner code than I do!

strong concept ACK!

renepickhardt · 2022-05-20T17:27:22Z

+    def __init__(self, path: list[Channel], amount: int = 0):
+        """Constructor method
+        """
+        self._paths = None


why is this plural?

renepickhardt · 2022-05-20T17:27:36Z

+        for channel in path:
+            if not isinstance(channel, Channel):
+                raise ValueError("path needs to be a collection of Channels")
+            self._path = path


what is the difference to self._paths from line 32?

this is the real one ;)

renepickhardt · 2022-05-20T17:30:27Z

+        self._amount = amount
+
+    def __str__(self):
+        return "Path with {} channels to deliver {} sats and status {}.".format(len(self._path),


Looking how I depict attempts in the evaluation we might want to change that string to include stuff like probability, fees, ppm. We could get inspiration by the printoutput of the simulation

renepickhardt · 2022-05-20T17:31:20Z

+        for attempt in attempts:
+            if not isinstance(attempt, Channel):
+                raise ValueError("path needs to be a collection of Channels")
+        self._paths = attempts


I really don't get this :(

this is indeed unnecessary as well.
This is part of the setter for path in Attempt. When I originally added the path to attempt, I used this setter. And while I refactored step by step, at the time i wasn't sure that didn't make any mistakes when passing on parameters. That's why i checked before inclusion and raised errors to see where things failed.

The setter now can go, because I made this a mandatory parameter at instantiation and I require the correct class.

Thanks for pointing this out!

renepickhardt · 2022-05-20T17:32:22Z

+        :return: estimated success probability before the attempt
+        :rtype: float
+        """
+        return self._probability


not entirely sure as the attempt class does not know the uncertainty network but we could compute this on the fly for the path (as the path contains such information)

there is btw a strong attempt against it as non disjoint paths will change probability computation.

yes, this is to collect statistical a priori information:
In _estimate_payment_statistics this is added when calling uncertainty_network.get_features_of_candidate_path. it's tied to this instance of attempt, so another Attempt on the same graph will have its own (then perhaps different probability). It should be purely descriptive and is not used for further calculation.

Bias in probabilities in UncertaintyGraph?

When collecting possible paths in a round/"payment loop", all path candidates are assumed to be sent and settled successfully.
This is my assumption, because we iterate over each attempt:

UncertaintyChannel.success_probability freshly calculates probabilities from current channel balances, and

then allocate_amount_on_path is updated before the loop commences with the next possibly not disjoint path, that then gets its probabilities from the just before adjusted inflight amount.

Or am I reading it wrong?

Ist this something we can live with?
What would be possible ways to correct for this (like trying to immediately figure out if the onion would fail or not)?

This indeed the really messy book keeping that I introduced. I am happy to find a better solution to this! I need to allocate the amounts for planned onions to have probability correction of the next path correct. (Ask me for an example if you need one!) I remove all of the allocated HTLCs then before I do the actual sending / probing. It is really the most ugly part of my code :(

just started to look at the code. I think this is btw exactly the difference between PLANNED and INFLIGHT while internally I increase the inflight counter I am actually in the planning phase and to compute the probabilities of planned paths I need to take into account that I already plan to allocate something sats to an edge (without knowing if that will work yet) So maybe we just need to move over the logic from the Enum to the state of a channel where we also have planned sats before we go and send onions and switch those to inflight sats. in this way I would note have to remove them before testing onions. does that make any sense? I am actually not sure myself if this would be any good.

renepickhardt · 2022-05-20T17:47:25Z

+        inflight_attempts = []
+        failed_attempts = []
+        print("\nStatistics about {} candidate onions:\n".format(len(attempts)))
+        for attempt in attempts:


I guess this is motivated by how the code was histroically structured. But wouldn't it make sense to have this attempts list as an sub_paymet = Payments() object and later in the loop just add he sub_payment to the payment?

this would allow us that we could later call something like sub_payment.filter(SettlementStatus.FAILED) instead of using an additional list

renepickhardt · 2022-05-20T17:48:58Z

+from pickhardtpayments import Channel
+
+
+class SettlementStatus(Enum):


AttemptStatus?

renepickhardt · 2022-05-20T17:51:36Z

-            payments[key]["erring_channel"] = erring_channel
+                attempt.path, attempt.amount)
            if success:
+                attempt.status = SettlementStatus.INFLIGHT


wouldn't that be AttmptStatus.ARRIVED?

renepickhardt · 2022-05-20T17:52:30Z

-                src, dest, amt, mu, base)
+            # transfer to a min cost flow problem and run the solver
+            # paths is the lists of channels, runtime the time it took to calculate all candidates in this round
+            attempts_in_round, runtime = self._generate_candidate_paths(


maybe sub_payment of type Payment

renepickhardt · 2022-05-20T17:54:06Z

-        end = time.time()
+        # When residual amount is 0 / enough successful onions have been found, then settle payment. Else drop onions.
+        if amt == 0:
+            for onion in payment.inflight_attempts:


technically inflight might not be at recipient yet and could still fail. So I guess we want ARRIVED here. I guess that was the confusion of this artificial distinction between INFLIGHT and ARRIVED

Yes. I think that indeed it is a lot clearer with ARRIVED.

…class

…lative paths on imports

renepickhardt

sry could not finnish review but wanted to at least drop the current comments

renepickhardt · 2022-05-28T06:24:36Z

+        :return: estimated success probability before the attempt
+        :rtype: float
+        """
+        return self._probability


just started to look at the code. I think this is btw exactly the difference between PLANNED and INFLIGHT while internally I increase the inflight counter I am actually in the planning phase and to compute the probabilities of planned paths I need to take into account that I already plan to allocate something sats to an edge (without knowing if that will work yet) So maybe we just need to move over the logic from the Enum to the state of a channel where we also have planned sats before we go and send onions and switch those to inflight sats. in this way I would note have to remove them before testing onions. does that make any sense? I am actually not sure myself if this would be any good.

renepickhardt · 2022-05-28T06:25:40Z

+    def __init__(self, path: list[UncertaintyChannel], amount: int = 0):
+        """Constructor method
+        """
+        self._routing_fee = -1


Didn't we say None instead of -1? Note that in particular one might have negative routing fees in the protocol in the future.

yes, negative routing fees will indeed break the -1 idea. I only changed it in payment, now changed it in attempt as well.
I would actually extract the planned/inflight problem into its own issue. i fear that discussion around it will get lost in this Thread. -> #25

…empt class

renepickhardt

Lets discuss out of band?

renepickhardt · 2022-06-21T12:41:41Z

+            self._probability *= channel.success_probability(amount)
+            # When Attempt is created, all amounts are set inflight. Needs to be updated with AttemptStatus change!
+            # This is to correctly compute conditional probabilities of non-disjoint paths in the same set of paths
+            # channel.in_flight(amount)


So far so good and makes sense

renepickhardt · 2022-06-21T12:47:58Z

+        """
+        if not self._status == value:
+            # remove allocated amounts when Attempt status changes from PLANNED
+            if self._status == AttemptStatus.PLANNED and not value == AttemptStatus.INFLIGHT:


Why not inflight?

I am wondering if we should have a setter API or if we can only inc status by 1.

That being said I see the idea. While i think it is -given the design of the code- the only reasonable option i think it demonstrates that I messed something up. (It feels really wrong to do the amounts tweeking here)

renepickhardt

Concept ACK. Thank you very much for this excellent work and for your endurance!

I think we are on the brink of merging this and refactoring the rest in #18 . However I had a few things where I have the feeling that the refactoring has not been executed fully. I hope my comments are helpful (as mentioned I have really poor concentration today)

Some of the comments don't need intermediate action but are just for the follow up PR but since we still need some tiny changes I would kindly ask you to also rebase against the main branch so that I can hopefully merge tomorrow (:

renepickhardt · 2022-06-22T03:45:47Z

-        except ValueError:
-            logging.warning("ValueError in Payment.filtered_attempts")
-        return []
+        return [attempt for attempt in self._attempts if attempt.status.value == flag.value]


Haha! That is not what I meant by creating a generstor using yield. But it is great to see how python can reduce "trivial" code blocks. (:

renepickhardt · 2022-06-22T03:53:23Z

Also LGTM!

adamritter · 2022-07-18T08:50:55Z

It seems like this change broke the payment algorithm or simulation. PickhardtPaymentsExample.ipynb examples don't work anymore:

Round number: 10
Try to deliver 18879883 satoshi:

Statistics about 46 candidate onions:

successful attempts:

p = 100.00% amt: 216543 sats hops: 4 ppm: 1413
p = 94.37% amt: 84263 sats hops: 5 ppm: 1349
p = 90.69% amt: 59164 sats hops: 5 ppm: 1708
p = 75.00% amt: 337453 sats hops: 5 ppm: 1417
p = 73.91% amt: 112351 sats hops: 5 ppm: 1374
p = 99.52% amt: 42718 sats hops: 6 ppm: 1348
p = 100.00% amt: 35946 sats hops: 6 ppm: 1367
p = 94.97% amt: 242846 sats hops: 7 ppm: 1696
p = 77.42% amt: 2351 sats hops: 9 ppm: 1427

failed attempts:

p = 100.00% amt: 88000 sats hops: 2 ppm: 1339
p = 74.25% amt: 288187 sats hops: 2 ppm: 1341
p = 14.57% amt: 1574652 sats hops: 2 ppm: 1359
p = 100.00% amt: 102400 sats hops: 2 ppm: 1381
p = 100.00% amt: 102400 sats hops: 2 ppm: 1356
....

SUMMARY:

Rounds of mcf-computations: 10
Number of attempts made: 448
Number of failed attempts: 356
Failure rate: 79.46%
total Payment lifetime (including inefficient memory management): 8.661 sec
Learnt entropy: 229.60 bits
fee for settlement of delivery: 0.000 sat --> 0 ppm
used mu: 100

…hardt#20) (renepickhardt#24)" This reverts commit cd46871.

sebulino added 13 commits May 13, 2022 15:46

add .gitignore

519185f

introduce register payment method on OracleLightningNetwork.py

cc1e862

adding settlement_payment in OracleLightningNetwork

4832253

first stub of a method as described in renepickhardt#16 Still needs to be tested

rewriting setter for actual_liquidity using properties (python way)

afa6a71

adjust channels in both directions when payment is made

4ecadee

Settlement of onions with partial payments only after all onions were…

a131eaf

… routed successfully

deletion of unused import

0b0e868

shell for a Payment Class

8dffa28

Revert "shell for a Payment Class"

8df2c57

This reverts commit 8dffa28.

shell for a Payment Class for issue renepickhardt#20

c03fe39

shell for as Attempt Class for issue renepickhardt#20

7170150

Indroduction of Payment and Attempt class, refactoring.

16bad26

Introduction of PaymentClass and AttemptClass, refactoring.

8727759

renepickhardt requested changes May 20, 2022

View reviewed changes

error fixing, semantic changes and refinement of Payment ant Attempt …

f6dad46

…class

sebulino requested a review from renepickhardt May 26, 2022 10:56

adding types in methods for Attempt and Payment Class and amending re…

40967ff

…lative paths on imports

renepickhardt reviewed May 29, 2022

View reviewed changes

sebulino mentioned this pull request May 29, 2022

probabilities in UncertaintyGraph #25

Open

sebulino added 2 commits May 29, 2022 13:22

initializes fee and probability w/ none (instead of -1) renepickhardt#20

5903cd2

adjustment of relative paths for modules in import

c1c61a0

sebulino requested a review from renepickhardt May 29, 2022 11:25

sebulino and others added 8 commits May 29, 2022 16:38

extending .gitignore

54e905c

Merge branch 'renepickhardt:main' into main

cf25aa0

Merge branch 'payments_attempts'

cf9c4f1

Merge remote-tracking branch 'origin/main'

9d56f65

correcting tipo for gitignore

3e69225

init for documentation

bf14533

configuration updated

97d01cb

correction in filter function for Attempt.Status renepickhardt#24

c8bfeb8

sebulino added 2 commits June 21, 2022 12:09

deleting docs folder and moving _estimate_payment_statistics into att…

29990a2

…empt class

removed doc folder

081d756

sebulino requested a review from renepickhardt June 21, 2022 10:22

renepickhardt reviewed Jun 21, 2022

View reviewed changes

adding CHANGELOG

c404105

sebulino requested a review from renepickhardt June 21, 2022 16:00

renepickhardt requested changes Jun 21, 2022

View reviewed changes

Cleaning up after Attempt and Payment PR review

3c850fe

sebulino force-pushed the payments_attempts branch from 2618078 to 62296de Compare June 21, 2022 21:49

sebulino requested a review from renepickhardt June 21, 2022 21:51

renepickhardt reviewed Jun 22, 2022

View reviewed changes

changed filter function in payment to generator

111a691

sebulino force-pushed the payments_attempts branch from 0ea743d to 111a691 Compare June 22, 2022 09:28

renepickhardt and others added 9 commits June 22, 2022 12:15

added setter to OracleChannel and fixed imports in init.py

c320189

Fix typos in documentation

de25111

rewriting setter for actual_liquidity using properties (python way)

40fb9be

Indroduction of Payment and Attempt class, refactoring.

54ccdc3

Introduction of PaymentClass and AttemptClass, refactoring.

a40e61a

adjustment of relative paths for modules in import

1a05a21

changing imports back to initial call

d494c0d

another rebasing hassle ;)

fc1e8d5

Merge branch 'main' into payments_attempts

8541a28

renepickhardt merged commit cd46871 into renepickhardt:main Jun 22, 2022

renepickhardt mentioned this pull request Jun 22, 2022

Introduction of a Path class #30

Open

sebulino deleted the payments_attempts branch June 24, 2022 13:26

adamritter mentioned this pull request Jul 19, 2022

Pull request #24 (Introduction of AttemptClass and PaymentClass) broke the payment algorithm or simulation #33

Open

adamritter added a commit to adamritter/pickhardtpayments that referenced this pull request Aug 5, 2022

Revert "Introduction of AttemptClass and PaymentClass (Issue renepick…

4b2a1b1

…hardt#20) (renepickhardt#24)" This reverts commit cd46871.

adamritter added a commit to adamritter/pickhardtpayments that referenced this pull request Aug 5, 2022

Revert "Introduction of AttemptClass and PaymentClass (Issue renepick…

e149e6b

…hardt#20) (renepickhardt#24)" This reverts commit cd46871.

		from pickhardtpayments import Channel


		class SettlementStatus(Enum):

Conversation

sebulino commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renepickhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebulino May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebulino May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Bias in probabilities in UncertaintyGraph?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renepickhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebulino May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renepickhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renepickhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebulino commented May 20, 2022 •

edited

Loading

sebulino May 20, 2022 •

edited

Loading

sebulino May 20, 2022 •

edited

Loading

sebulino May 29, 2022 •

edited

Loading